fix(builtins): honor --help argv order and rewrite pflag errors to GNU style#238
fix(builtins): honor --help argv order and rewrite pflag errors to GNU style#238
Conversation
…U style Two follow-ups from PR #205 (df builtin) review: 1. `cmd --help --bogus` now prints help and exits 0, matching GNU coreutils' left-to-right `--help` short-circuit. Previously pflag scanned the whole argv and failed on `--bogus` first. Implemented in `builtins/builtins.go` via a pre-parse args trim, scoped to commands that register a `help` flag. 2. pflag's default flag-error wording (`unknown flag: --foo`, `unknown shorthand flag: 'X' in -...`, `flag does not allow an argument`, `flag needs an argument: --foo`) is now translated to GNU's `unrecognized option '--foo'`, `invalid option -- 'X'`, `option '--foo' doesn't allow an argument`, and `option '--foo' requires an argument`. A `Try 'cmd --help' for more information.` footer is appended when the command exposes `--help`. Several df scenarios that previously carried `skip_assert_against_bash` solely because of wording divergence now match GNU byte-for-byte and have been de-skipped. Updated all sibling scenarios and Go tests (ping, pwd, ip, strings, uniq, sed, du) that asserted on the old pflag wording. Refs: - #205 (comment) - #205 (comment) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25059712c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex review caught that `df --no-such=value` emitted `df: unrecognized option '--no-such'` while GNU emits the full token `'--no-such=value'`. pflag strips the `=value` suffix from its error string, so `rewritePflagError` now takes the parsed argv and scans it to recover the original token. Added a unit-test case (--no-such=value, --no-such=, and `--` boundary) plus a scenario test that asserts byte-for-byte match against GNU. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c85098992
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex caught a second wording divergence: pflag emits two different
payloads for a missing argument depending on flag form:
long form: `flag needs an argument: --foo`
short form: `flag needs an argument: 'X' in -Y`
Previously the rewriter inlined the payload into a single template,
producing `option ''n' in -n' requires an argument` for `head -n`.
GNU uses two separate templates:
long: `option '--foo' requires an argument`
short: `option requires an argument -- 'X'`
`rewritePflagError` now branches on the payload shape via a small
`shortMissingArg` helper. Added unit-test cases for the single
shorthand and the combined-shorthand form (`-an`) plus scenario tests
under `tests/scenarios/cmd/head/errors/missing_arg_{short,long}.yaml`
that assert against GNU.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb77a039a0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged that the previous trim "args at first --help" was too aggressive: `head -n nope --help --bogus` would discard `--bogus`, let pflag parse `-n nope --help` cleanly, then short-circuit on --help and exit 0 — silently swallowing both the unknown later flag and the invalid earlier value. GNU instead surfaces the earlier validation error. Restricted the trim to the case where `--help` is the very first argument. This still covers the cases the original reviewer cited (`df --help --sync`, `df --help --bad`) and every existing test, but keeps full pflag validation when `--help` appears after another flag — preserving GNU's "fail on the leftmost bad option" semantics. Added a regression scenario at `tests/scenarios/cmd/head/errors/help_after_value_does_not_swallow_unknown_flag.yaml` that locks in: `head -n nope --help --bogus` exits 1 (does not print help). skip_assert_against_bash is set because GNU's failure reason (invalid -n value) and rshell's (unknown --bogus) differ — both exit 1 and neither prints help, which is the property we care about. The underlying value-validation gap is a pre-existing issue out of scope. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
GNU getopt parses `-h=true` as the no-arg shorthand `-h` followed by `=` interpreted as another shorthand char (which isn't registered), emitting `df: invalid option -- '='`. pflag instead treats `=` as the value separator and routes the value through Set, which our no-arg guards reject. Detect the `-X=value` argv shape in the rewriter and substitute GNU's wording when the descriptor's shorthand char matches X. Closes the last wording-only divergence flagged in PR #205's follow-up audit (`tests/scenarios/cmd/df/flags/rejected_h_equals_true.yaml`); skip removed and stderr now matches GNU byte-for-byte. Updated unit tests: - new case: -h=true with descriptor `-h, --human-readable` → `'='` - existing -k=true case (short-only descriptor) updated — GNU also emits `invalid option -- '='` for that, not `option '-k' doesn't allow an argument` Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55983930b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codex flagged that GNU honors --help even after earlier valid options — `head -q --help --bogus` and `df -h --help --bogus` print help and exit 0. The previous "first-only" trim was too narrow. The new `safeHelpTrimIndex` scans tokens before --help and trims only when every preceding token is a registered no-argument flag (long form or shorthand cluster). Tokens that could fail validation (value-takers, =value forms, positionals, unknown flags, end-of-flags `--`) all preclude the trim — keeping the earlier-failure semantics from the prior round (`head -n nope --help --bogus` still errors instead of silently swallowing the invalid value). Added unit tests for `safeHelpTrimIndex` covering each token class and a regression scenario at `tests/scenarios/cmd/df/basic/help_short_circuits_after_no_arg_flag.yaml` (`df -h --help --bogus` exits 0 with usage). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CI's `Fuzz (pwd)` and `Fuzz (du)` jobs caught a panic in the new `isNoArgFlagToken`: for a token like `-˞`, the loop calls `fs.ShorthandLookup(string(a[i]))`. When `a[i]` is a UTF-8 continuation byte (≥ 0x80), `string(byte)` produces the 2-byte UTF-8 encoding of that byte interpreted as a rune (e.g. 0xCB → "Ë"), and pflag panics with `can not look up shorthand which is more than one ASCII character`. The interpreter recovers and reports `internal error` from api.go's panic guard. Guard the loop: any byte outside printable ASCII (≤ ' ' or > 0x7E) disqualifies the token as a no-arg cluster — non-ASCII bytes can't be valid shorthand chars anyway. Captured the failing input as a permanent corpus seed (`builtins/pwd/testdata/fuzz/FuzzPwdArgs/9386e59311458487`) and added a unit-test case to TestSafeHelpTrimIndex. Verified locally: 30s fuzz on both pwd and du now passes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18e763ab39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !isNoArgFlagToken(fs, a) { | ||
| return 0, false |
There was a problem hiding this comment.
For commands with value-taking flags, this rejects trimming as soon as a valid value option appears before --help, so head -n 5 --help --bogus is left for pflag to parse and fails on the later --bogus. GNU head -n 5 --help --bogus exits 0 and prints help because the earlier -n 5 is valid and --help short-circuits before later options; this means the new left-to-right help behavior is only correct for no-arg flags and regresses valid invocations of head/tail-style builtins.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
[Claude Opus 4.7 (1M context)] You're right that GNU prints help here, but extending the trim to allow value-takers conflicts with the prior round's feedback (3202120242) about head -n nope --help --bogus — pflag accepts nope as a String value but head's numeric validation runs only after --help short-circuits, so trimming would silently swallow the validation failure. GNU itself diverges by value validity:
head -n 5 --help --bogus→ exit 0, prints help (-n 5 is valid)head -n nope --help --bogus→ exit 1,invalid number of lines: 'nope'
Verified both with docker run --rm debian:bookworm-slim. To match GNU on both, head's handler would need to validate -n's numeric value via pflag.Var.Set before the --help short-circuit fires — that's a per-builtin handler-ordering change, not something the shared trim in builtins/builtins.go can resolve safely (we don't know which value-taker fields a builtin's handler validates internally).
Trade-off matrix with the current no-arg-only trim:
| invocation | GNU | rshell now | extending trim |
|---|---|---|---|
-q --help --bogus (no-arg) |
help, 0 | help, 0 ✓ | help, 0 ✓ |
-n 5 --help --bogus |
help, 0 | error, 1 | help, 0 ✓ |
-n nope --help --bogus |
error, 1 | error, 1 ✓ | help, 0 ✗ |
Two out of three match either way; extending makes the -n 5 case match at the cost of regressing the nope case. Deferring to a follow-up PR that re-orders head/tail/etc. handlers to validate values before --help short-circuits, which closes both gaps cleanly. Leaving this thread open for the reviewer to decide between the trade-off.
There was a problem hiding this comment.
Will do in another PR
julesmcrt
requested review from
AlexandreYang,
astuyve,
matt-dz,
thieman and
val06
as code owners
| // flag does not allow an argument` an argument" | ||
| // | ||
| // Unknown messages are returned unchanged. | ||
| func rewritePflagError(err error, args []string) string { |
There was a problem hiding this comment.
Hum,
1/ I'm not sure if patching pflag is the right way. Maybe we should just re-code our own flag parser. WDYT?
2/ I think we should at least move all the flag parsing logic to a separate package like /builtins/internal/flagparser/ to isolate flag parsing logic. Having all this flag parsing logic in builtins.go seems not ideal.
And then, we can optionally implement 1/ more easy if needed.
2 participants
Summary
Two follow-ups from PR #205 (df builtin) review feedback:
cmd --help --bogusnow prints help and exits 0, matching GNU coreutils' left-to-right--helpshort-circuit. Previously pflag scanned the whole argv and failed on--bogusfirst. Implemented inbuiltins/builtins.govia a pre-parse args trim, scoped to commands that register ahelpflag.pflag flag-error wording now matches GNU getopt byte-for-byte:
unknown flag: --foo→unrecognized option '--foo'unknown shorthand flag: 'X' in -...→invalid option -- 'X'flag does not allow an argument→option '--foo' doesn't allow an argumentflag needs an argument: --foo→option '--foo' requires an argumentTry 'cmd --help' for more information.footer appended when the command exposes--help.Several
dfscenarios that previously carriedskip_assert_against_bashsolely because of wording divergence now match GNU byte-for-byte and have been de-skipped. Sibling scenarios and Go tests for ping, pwd, ip, strings, uniq, sed, and du were updated to the new wording.Refs:
Test plan
🤖 Generated with Claude Code